Merge builtins.wasm and builtins.wasi into one function#370
Merged
Conversation
The new interface is
builtins.wasm {
path = ./path/to/file.wasm;
wasi = false;
function = "fib";
} 33
📝 WalkthroughWalkthroughConsolidates Wasm handling into a single primop ( Changes
Sequence Diagram(s)sequenceDiagram
participant Evaluator as Nix Evaluator
participant Parser as Config Parser
participant Instantiator as Wasm Instantiator
participant WasiRuntime as WASI Runtime
participant NonWasiRuntime as Non‑WASI Runtime
participant Module as Wasm Module
Evaluator->>Parser: __wasm(config, arg)
Parser->>Instantiator: validate config (path, function?)
Instantiator->>Module: load & compile module
Instantiator->>Instantiator: inspect imports -> detect `_start` => useWasi?
alt useWasi == true
Instantiator->>WasiRuntime: instantiate with WASI, wire return_to_nix
WasiRuntime->>Module: call _start(argv[1])
Module-->>WasiRuntime: invoke return_to_nix(result)
WasiRuntime-->>Evaluator: deliver result
else useWasi == false
Instantiator->>NonWasiRuntime: instantiate without WASI
NonWasiRuntime->>Module: call nix_wasm_init_v1()
NonWasiRuntime->>Module: call configured function(valueId)
Module-->>NonWasiRuntime: return i32 valueId
NonWasiRuntime-->>Evaluator: deliver result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Member
|
Maybe we should make the toggle not a boolean but an enum, like |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
doc/manual/source/protocols/wasm.md (1)
38-41: Clarifynix_wasm_init_v1()call frequency wording.“called once when the module is instantiated” can read as global/per-module caching behavior. Consider clarifying it is once per instance (i.e., per
builtins.wasminvocation).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/manual/source/protocols/wasm.md` around lines 38 - 41, Clarify the wording for nix_wasm_init_v1() to state it is invoked once per module instance, not globally; update the sentence mentioning `nix_wasm_init_v1()` to read that the function is called once for each instantiation of the module (i.e., once per `builtins.wasm` invocation / per instance), so readers understand it’s per-instance initialization rather than a single global call across the process.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/libexpr/primops/wasm.cc`:
- Around line 614-635: The WASI configuration is being applied after
instantiateWasm(), but Wasmtime requires setting the WASI context on the store
before module instantiation; move the WasiConfig creation and calls to
wasi_config_set_stdout_custom/wasi_config_set_stderr_custom,
WasiConfig::argv(...) and the call to instance.wasmStore.context().set_wasi(...)
to occur before calling instantiateWasm(), then call instantiateWasm(state,
wasmPath, useWasi) (and only afterward use instance.addValue and
instance.getExport<Func>().call(...)); ensure the WasiLogger, its trampoline,
and argv are created and passed into the store via set_wasi prior to
instantiation so the module start can safely use WASI.
---
Nitpick comments:
In `@doc/manual/source/protocols/wasm.md`:
- Around line 38-41: Clarify the wording for nix_wasm_init_v1() to state it is
invoked once per module instance, not globally; update the sentence mentioning
`nix_wasm_init_v1()` to read that the function is called once for each
instantiation of the module (i.e., once per `builtins.wasm` invocation / per
instance), so readers understand it’s per-instance initialization rather than a
single global call across the process.
Collaborator
Author
|
@grahamc It now auto-detects whether to use WASI. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/libexpr/primops/wasm.cc (1)
607-642:⚠️ Potential issue | 🔴 CriticalConfigure WASI before instance instantiation.
Line 607 instantiates the module before Line 641 calls
set_wasi. If the module executes start-time code during instantiation, this ordering can fail before WASI is available. This appears to be the same unresolved issue raised earlier.In Wasmtime C++/C APIs, should `Store::Context::set_wasi(...)` be called before `instantiate` when start-time code may run?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libexpr/primops/wasm.cc` around lines 607 - 642, The code currently calls instantiateWasm before configuring WASI which can run module start-time code without WASI available; move the WASI setup so that when instance.pre->useWasi is true you create a WasiConfig, set the custom stdout/stderr trampolines and argv, call instance.wasmStore.context().set_wasi(std::move(wasiConfig)) before calling instantiateWasm, and then call instantiateWasm (and remove the later set_wasi call); look for instantiateWasm, instance.pre->useWasi, WasiConfig, wasi_config_set_stdout_custom/wasi_config_set_stderr_custom, and instance.wasmStore.context().set_wasi to make these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/manual/source/protocols/wasm.md`:
- Line 355: Update the stale wording that says "wasi = true" to instead
reference WASI mode detection: change the sentence "This function is only
available when `wasi = true`" to something like "This function is only available
in WASI mode (module exports `_start`)" so it matches the document's
auto-detection behavior and references the `_start` export.
In `@src/libexpr/primops/wasm.cc`:
- Around line 98-103: The current heuristic that marks useWasi = true by
scanning module.exports() for the "_start" export is incorrect; instead detect
WASI by checking module.imports() for a WASI import module (e.g.,
"wasi_snapshot_preview1", "wasi_unstable", or imports whose module name starts
with "wasi_") and set useWasi based on that. Replace the export-based loop that
inspects module.exports() and the "_start" name with a loop over
module.imports() (or equivalent API) that looks at import.module() (or
import.module_name()) for known WASI module names and sets useWasi = true if
found, leaving all other logic (including the code that validates the "function"
attribute) untouched. Ensure you reference the same variables (useWasi, module)
and remove reliance on "_start" so WASI reactors and command modules are both
detected correctly.
---
Duplicate comments:
In `@src/libexpr/primops/wasm.cc`:
- Around line 607-642: The code currently calls instantiateWasm before
configuring WASI which can run module start-time code without WASI available;
move the WASI setup so that when instance.pre->useWasi is true you create a
WasiConfig, set the custom stdout/stderr trampolines and argv, call
instance.wasmStore.context().set_wasi(std::move(wasiConfig)) before calling
instantiateWasm, and then call instantiateWasm (and remove the later set_wasi
call); look for instantiateWasm, instance.pre->useWasi, WasiConfig,
wasi_config_set_stdout_custom/wasi_config_set_stderr_custom, and
instance.wasmStore.context().set_wasi to make these changes.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/libexpr/primops/wasm.cc (1)
135-144:⚠️ Potential issue | 🟠 MajorWASI context is still configured after module instantiation.
The
NixWasmInstanceconstructor instantiates the module at line 140 viapre->instancePre.instantiate(wasmCtx), butset_wasi()is called much later at line 641 inprim_wasm. While usinginstantiate_prepre-compiles the module, the actual instantiation still happens in the constructor before WASI is configured. If the Wasm module has a start section (distinct from the_startexport), it will execute during instantiation and may call WASI functions before the context is set, causing an abort.Consider restructuring so that
set_wasi()is called on the store context beforeNixWasmInstanceis constructed, or defer instantiation until after WASI configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libexpr/primops/wasm.cc` around lines 135 - 144, The constructor NixWasmInstance currently calls pre->instancePre.instantiate(wasmCtx) (in NixWasmInstance(...)) before WASI is configured via set_wasi (done later in prim_wasm), which allows a module start function to run without a proper WASI context; fix by moving instantiation to after WASI is set: either (A) call set_wasi on the store/context before constructing NixWasmInstance so the wasmCtx passed into NixWasmInstance already has WASI, or (B) change NixWasmInstance to defer instantiate (remove/replace instance = unwrap(pre->instancePre.instantiate(wasmCtx)) from the constructor and add an explicit instantiate() method invoked from prim_wasm after set_wasi completes); update all call sites in prim_wasm to construct NixWasmInstance first only when using approach A or to call the new instantiate() method after set_wasi when using approach B, ensuring wasmCtx.set_data(this) is retained on the correct store/context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/manual/source/protocols/wasm.md`:
- Around line 43-52: The "WASI Mode" section currently says WASI is detected by
the module exporting `_start`, which contradicts the overview and non-WASI
section; update the "WASI Mode" text to state that WASI is detected by the
presence of `wasi_snapshot_preview1` imports, and optionally note that an
exported `_start` is just the conventional entrypoint used by WASI runtimes (but
not the detection mechanism). Adjust any example or wording in the "WASI Mode"
heading to reference `wasi_snapshot_preview1` as the detection mechanism and
keep `_start` described only as the entry point.
In `@src/libexpr/primops/wasm.cc`:
- Around line 671-701: The docstring for the "__wasm" primop (registered via
primop_wasm with implementation prim_wasm) inaccurately says WASI mode is
detected by checking for an exported "_start"; update the documentation to match
the implementation: state that WASI mode is detected by the presence of wasi
imports (e.g. "wasi_snapshot_preview1") rather than an exported "_start", and
clarify the rule about when the `function` attribute is required/forbidden
accordingly so the text aligns with how prim_wasm actually detects WASI.
- Around line 612-622: The error text thrown when instance.pre->useWasi is true
is misleading because WASI is detected via imports (wasi_snapshot_preview1) not
the presence of a '_start' export; update the Exception message thrown in the
builtins.wasm handling (the throw inside the if (instance.pre->useWasi) path
where functionAttr is provided) to reference WASI imports (e.g., "WASI modules
(modules that import 'wasi_snapshot_preview1') do not accept a 'function'
attribute") or similar wording that mentions imports rather than a '_start'
export; keep the same throw location and type (throw Error(...)) and ensure any
other related messages around functionAttr handling in this block (and the
non-WASI branch) remain consistent.
---
Duplicate comments:
In `@src/libexpr/primops/wasm.cc`:
- Around line 135-144: The constructor NixWasmInstance currently calls
pre->instancePre.instantiate(wasmCtx) (in NixWasmInstance(...)) before WASI is
configured via set_wasi (done later in prim_wasm), which allows a module start
function to run without a proper WASI context; fix by moving instantiation to
after WASI is set: either (A) call set_wasi on the store/context before
constructing NixWasmInstance so the wasmCtx passed into NixWasmInstance already
has WASI, or (B) change NixWasmInstance to defer instantiate (remove/replace
instance = unwrap(pre->instancePre.instantiate(wasmCtx)) from the constructor
and add an explicit instantiate() method invoked from prim_wasm after set_wasi
completes); update all call sites in prim_wasm to construct NixWasmInstance
first only when using approach A or to call the new instantiate() method after
set_wasi when using approach B, ensuring wasmCtx.set_data(this) is retained on
the correct store/context.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
doc/manual/source/protocols/wasm.md (1)
45-45:⚠️ Potential issue | 🟡 MinorInconsistent wording for WASI detection mechanism.
Line 45 uses "imports a
wasi_snapshot_preview1function" while lines 15 and 28 consistently use "imports fromwasi_snapshot_preview1". The latter phrasing is more accurate sincewasi_snapshot_preview1is the module name from which WASI functions are imported.📝 Suggested fix for consistency
-WASI mode is automatically used when the module imports a `wasi_snapshot_preview1` function. +WASI mode is automatically used when the module imports from `wasi_snapshot_preview1`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/manual/source/protocols/wasm.md` at line 45, Replace the inconsistent phrasing "imports a `wasi_snapshot_preview1` function" with the consistent and accurate wording "imports from `wasi_snapshot_preview1`" so the sentence reads: "WASI mode is automatically used when the module imports from `wasi_snapshot_preview1`." Update the occurrence matching that exact phrase to match lines that use "imports from `wasi_snapshot_preview1`" elsewhere in the document.
🧹 Nitpick comments (1)
doc/manual/source/protocols/wasm.md (1)
38-41: Add function signature fornix_wasm_init_v1().Line 40 introduces the
nix_wasm_init_v1()requirement but doesn't specify its signature. For consistency with line 41 (which specifies the entry point signature asfn(arg: u32) -> u32), consider adding the signature fornix_wasm_init_v1().📋 Suggested enhancement
-Every Wasm module used in non-WASI mode must export: -- A `memory` object that the host can use to read/write data. -- `nix_wasm_init_v1()`, a function that is called once when the module is instantiated. -- The entry point function, whose name is specified by the `function` attribute. It takes a single `ValueId` and returns a single `ValueId` (i.e. it has type `fn(arg: u32) -> u32`). +Every Wasm module used in non-WASI mode must export: +- A `memory` object that the host can use to read/write data. +- `nix_wasm_init_v1()`, a function with signature `fn()` (no parameters or return value) that is called once when the module is instantiated. +- The entry point function, whose name is specified by the `function` attribute. It takes a single `ValueId` and returns a single `ValueId` (i.e. it has type `fn(arg: u32) -> u32`).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/manual/source/protocols/wasm.md` around lines 38 - 41, Add a clear signature for the required init function: state that nix_wasm_init_v1 must be exported with the signature fn() -> u32 (or specify the exact parameter/return types expected) so it matches the style used for the entry point; update the sentence mentioning nix_wasm_init_v1 to explicitly list its function signature (referencing the symbol nix_wasm_init_v1 and contrast with the entry point named by the function attribute) and ensure the documentation still lists memory and the entry point signature fn(arg: u32) -> u32.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@doc/manual/source/protocols/wasm.md`:
- Line 45: Replace the inconsistent phrasing "imports a `wasi_snapshot_preview1`
function" with the consistent and accurate wording "imports from
`wasi_snapshot_preview1`" so the sentence reads: "WASI mode is automatically
used when the module imports from `wasi_snapshot_preview1`." Update the
occurrence matching that exact phrase to match lines that use "imports from
`wasi_snapshot_preview1`" elsewhere in the document.
---
Nitpick comments:
In `@doc/manual/source/protocols/wasm.md`:
- Around line 38-41: Add a clear signature for the required init function: state
that nix_wasm_init_v1 must be exported with the signature fn() -> u32 (or
specify the exact parameter/return types expected) so it matches the style used
for the entry point; update the sentence mentioning nix_wasm_init_v1 to
explicitly list its function signature (referencing the symbol nix_wasm_init_v1
and contrast with the entry point named by the function attribute) and ensure
the documentation still lists memory and the entry point signature fn(arg: u32)
-> u32.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
doc/manual/source/protocols/wasm.mdsrc/libexpr/primops/wasm.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- src/libexpr/primops/wasm.cc
grahamc
approved these changes
Feb 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The new interface is
This will allow other Wasm-related configuration to be added in the future (e.g. WASI features, memory limits, caching info, etc).
The argument is separate from the configuration because typically you want to write something like
It now also auto-detects whether a module uses WASI.
Context
Summary by CodeRabbit
Documentation
Refactor